chore: rate limit peer exchange protocol, enhanced response status in RPC #3035
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NagyZoltanPeter
changed the title
|
You can find the image built from this PR at Built from cf4e167 |
NagyZoltanPeter
requested review from
jm-clius,
Ivansete-status,
darshankabariya,
gabrielmer and
fryorcraken
…s and desc to the rpc
…sible to set separate limits per protocol. Adjusted mountings. Added and adjusted tests
NagyZoltanPeter
force-pushed
the
chore-rate-limit-peer-ex
branch
from
210332a to
4b99834
Compare
gabrielmer
approved these changes
Sep 16, 2024
waku/common/rate_limit/setting.nim
Outdated
Comment on lines
44
to
60
| proc translate(sProtocol: string): RateLimitedProtocol = | ||
| if sProtocol.len == 0: | ||
| return GLOBAL | ||
|
|
||
| case sProtocol | ||
| of "global": | ||
| return GLOBAL | ||
| of "storev2": | ||
| return STOREV2 | ||
| of "storev3": | ||
| return STOREV3 | ||
| of "lightpush": | ||
| return LIGHTPUSH | ||
| of "px": | ||
| return PEEREXCHG | ||
| of "filter": | ||
| return FILTER |
There was a problem hiding this comment.
should we handle the case of a string being passed that doesn't match any of the protocols?
There was a problem hiding this comment.
Mmm, yes it worth a double check. Originally invalid strings are filtered with regex.
Ivansete-status
approved these changes
Sep 16, 2024
| case sProtocol | ||
| of "global": | ||
| return GLOBAL | ||
| of "storev2": |
There was a problem hiding this comment.
I wonder if we should instead use the const items that we already have, such as:
…C is changed the now respond structure will contain status_code and status_desc.
|
Latest changes done on the peer-exchange specification is reflected now in nwaku code. |
jm-clius
approved these changes
Sep 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
As part of DOS protection for req-res protocols and metrics Waku PeerExchange protocol must be protected just as Lightpush/Store and Filter.
Changes
Warning
The last bullet point introduces a refactored rate limit configuration over CLI/TOML file.
This was necessary as identified different needs for different protocols (out of dogfooding and dashboard data)
This change will break former CLI as removed two former rate-limit option and replaced them by one - formatted string - that can be repeated.
New CLI option is
--rate-limitwhich has to be set as string in a proper format.Examples:
--rate-limit="lightpush:13/10s"--rate-limit="storev3:15/1500ms"--rate-limit="px:1/1m"--rate-limit="100/10m"As this option can be repeated rate limit configuration can be fine-grained for every needs.
How to test
Tests are adjusted and extended. Every peer exchange test must run without error.
Issue
#3028
Further actions: